Adding additional command line param of sync-access-token #4199
Conversation
|
@soninaren do you mind taking a look at this |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new command-line parameter (--sync-access-token) to allow the sync triggers call to use an access token that differs from the normal one. Key changes include updating the sync triggers logic in PublishFunctionAppAction.cs to use the new token if provided and adding the new SyncAccessToken property and parser setup in BaseAzureAction.cs.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Azure.Functions.Cli/Actions/AzureActions/PublishFunctionAppAction.cs | Updated the sync triggers method to assign SyncAccessToken from AccessToken if not provided and then use it in the sync call. |
| src/Azure.Functions.Cli/Actions/AzureActions/BaseAzureAction.cs | Added a new SyncAccessToken property and command-line parameter for it. |
Comments suppressed due to low confidence (2)
src/Azure.Functions.Cli/Actions/AzureActions/PublishFunctionAppAction.cs:651
- Consider adding unit tests for the sync triggers functionality to cover cases where 'SyncAccessToken' is provided and when it falls back to using 'AccessToken'.
if (string.IsNullOrEmpty(SyncAccessToken))
src/Azure.Functions.Cli/Actions/AzureActions/BaseAzureAction.cs:43
- Consider adding unit tests to validate the new '--sync-access-token' command-line parameter, including its default handling when no sync token is explicitly provided.
Parser.Setup<string>("sync-access-token")
| .Callback(t => AccessToken = t); | ||
| Parser | ||
| .Setup<string>("sync-access-token") | ||
| .WithDescription("Access token to use for performing trigger sync azure action (if different to normal)") |
There was a problem hiding this comment.
nit: remove (if different to normal). This might cause some confusion for customers as what makes access token different.
| HttpResponseMessage response = null; | ||
| // This SyncTriggers function calls the endpoint for linux syncTriggers | ||
| response = await AzureHelper.SyncTriggers(functionApp, AccessToken, ManagementURL); | ||
| if (string.IsNullOrEmpty(SyncAccessToken)) |
There was a problem hiding this comment.
Can we do the inverse instead and leave rest of the code as is.
if (!string.IsNullOrEmpty(SyncAccessToken))
{
AccessToken = SyncAccessToken;
}
|
Sorry it's taken so long to get to this. The App Service Authentication layer shouldn't come into play here. The Core Tools calls ARM, which in turn calls an app admin endpoint which is not subject to user-defined auth checks. |
If that's the case, then there are some issues with the kudu deployment process (would need to go back and check my notes on the issue I was hitting at the time). Deployments behind fully authentication enabled do fail to correctly sync due to these calls not having the correct Auth passed. |
3 participants
Issue describing the changes in this PR
Related to the issue linked below, adds the ability to pass a different access token to be used for the sync triggers call.
partially resolves #4196
Pull request checklist